Fix: initialise dep_gen on the orchestrator, not behind the handshake - #1486
Conversation
dep_gen_aicpu_init() resolves the collector's buffer state and pops its
first buffer, and it ran from the scheduler cold path after the AICore
handshake, next to pmu_aicpu_init(). pmu belongs there — it needs the
physical_core_ids_ the handshake produces. dep_gen needs nothing from
it: init only reads dep_gen_data_base, which kernel.cpp publishes before
aicpu_execute() starts.
The orchestrator, meanwhile, deliberately skips that handshake:
if (decouple_orch && is_orchestrator) {
return 0; // do NOT touch the handshake or hs_arrived_ barrier
}
so it goes straight to building the graph while the schedulers are still
handshaking cores. Every submit_task it issues in that window reaches
dep_gen_aicpu_record_submit() with s_dep_gen_state still null and is
dropped. Whether any records survive is a race against handshake wall
time, which scales with core count: at three clusters the schedulers
usually win, at a full device they usually do not, and in between runs
capture a partial graph missing only its first tasks.
Initialising dep_gen on the thread that feeds it removes the race —
there is nothing left to order it against. Both cold-path call sites go,
not just one: the free_queue is SPSC and the orchestrator is now its
only device-side consumer.
The failure was silent because the early return skipped the accounting
one line below it, the line whose comment reads "Account every attempted
record so total == collected + dropped on host". The host's
reconcile_counters() saw 0/0/0, called that consistent, and wrote an
empty deps.json instead of refusing. A pre-init record cannot be
accounted — the counter lives in the BufferState that is still null — so
it now reports itself once instead. Once, because record_submit is a
per-task path and the AICPU log is not free.
a5 carried the identical arrangement and is fixed with it.
📝 WalkthroughWalkthroughDepGen initialization is moved from scheduler cold paths into the orchestrator flow for both runtime variants. The collector now detects pre-initialization submissions and emits a one-time error before dropping them. ChangesDepGen sequencing and pre-init handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SchedulerContext
participant AicpuExecutor
participant DepGenCollector
SchedulerContext-->>AicpuExecutor: Complete post-handshake setup without DepGen initialization
AicpuExecutor->>DepGenCollector: dep_gen_aicpu_init()
AicpuExecutor->>DepGenCollector: dep_gen_aicpu_set_orch_thread_idx(thread_idx)
AicpuExecutor->>DepGenCollector: Submit dependency record
DepGenCollector-->>AicpuExecutor: Record or one-time pre-init error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp (1)
1225-1225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the obsolete scheduler-side DepGen guards.
DepGen initialization now belongs exclusively to the orchestrator path, so these empty conditionals add dead control flow and leave misleading scheduler comments.
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L1225-L1225: remove the empty guard and update the surrounding initialization comment.src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L984-L984: remove the empty profiling guard.src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L1221-L1221: remove the empty guard and update the surrounding initialization comment.src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp#L976-L976: remove the empty profiling guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp` at line 1225, Remove the obsolete scheduler-side DepGen guards: in src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp at lines 1225 and 984, and src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp at lines 1221 and 976, delete the empty guards, remove the empty profiling guards, and update each surrounding initialization comment to reflect that DepGen initialization is handled exclusively by the orchestrator path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp`:
- Line 1225: Remove the obsolete scheduler-side DepGen guards: in
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
at lines 1225 and 984, and
src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
at lines 1221 and 976, delete the empty guards, remove the empty profiling
guards, and update each surrounding initialization comment to reflect that
DepGen initialization is handled exclusively by the orchestrator path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39545b0b-c3f2-4243-b443-d0eea975496c
📒 Files selected for processing (5)
src/a2a3/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a5/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/common/platform/shared/aicpu/dep_gen_collector_aicpu.cpp
The bug
The
dep_genDFX collector intermittently emits an emptydeps.json:got set()— no edges at all, rather than wrong edges. The records are neverrecorded, not buffered-and-unread and not dropped at emit.
dep_gen_aicpu_init()resolves the collector's buffer state and pops its firstbuffer, and it ran from the scheduler cold path after the AICore handshake,
next to
pmu_aicpu_init(). pmu belongs there — it needs thephysical_core_ids_the handshake produces. dep_gen needs nothing from it: init only reads
dep_gen_data_base, whichkernel.cpppublishes beforeaicpu_execute()evenstarts.
The orchestrator, meanwhile, deliberately skips that handshake
(
aicpu_executor.cpp):so it goes straight to building the graph while the schedulers are still
handshaking cores. Every
submit_taskin that window reachesdep_gen_aicpu_record_submit()withs_dep_gen_statestill null and is dropped.Instrumented device log, same binary, same card, only the width changed:
It is a race, not a capacity limit
Handshake wall time scales with core count, so the wider the device the more
often the orchestrator wins:
| block_dim | 2 | 3 | 4 | 6 | 8 | 12 | 20 | 24 |
| --- | --- | --- | --- | --- | --- | --- | --- |
| pass rate | 2/2 | 5/5 | 2/2 | 1/2 | 3/4 | 0/5 | 1/5 | 1/5 |
There is no threshold and no 21/22 cliff — I looked for one, since #1477 had
just fixed a 21-cluster bound in
CoreTracker, and this is not that. Theintermediate runs are the giveaway: several failed with
tasks[] missing expected ids: {0}, i.e. the graph captured except its very first submit. Nosizing or indexing bug loses exactly the first record and keeps the rest.
This already reaches CI on
main. The CI runner loses the race even atblock_dim: 3—st-onboard-a2a3on #1478, which does not touchblock_dimatall, fails with exactly this assertion, and
main's own history has anst-onboard-a2a3failure among recent runs.Why only dep_gen
pmu/args_dump/l2_swimlaneare per-core subsystems whose producers aredownstream of the same handshake that gates their init, so they cannot race it.
scope_statsis orchestrator-side like dep_gen, but resolves its pointers insideset_platform_scope_stats_base()— beforeaicpu_execute()— and pops itsbuffer lazily. dep_gen was the only orchestrator-side subsystem with an eager
init parked behind the handshake. I ran all five per-feature smokes at full
width: only dep_gen failed.
The fix
Initialise dep_gen on the thread that feeds it, immediately before the existing
set_orch_thread_idx()and beforeorch_func_runs. There is then nothing leftto order it against.
Both cold-path call sites go, not just one: the free_queue is SPSC and the
orchestrator must be its only device-side consumer. a5 carried the identical
arrangement and is fixed with it.
The silence
The failure surfaced as an empty file rather than an error because the early
return skipped the accounting one line below it — the line whose comment reads
"Account every attempted record so total == collected + dropped on host". The
host's
reconcile_counters()saw0/0/0, called that consistent, and wrote{"tasks":[],"tensors":[],"edges":[]}.A pre-init record cannot be accounted — the counter lives in the
BufferStatethat is still null — so it now reports itself once instead. Once, because
record_submitis a per-task path and the AICPU log is not free(
.claude/rules/codestyle.md§7).Verification
pytest examples tests/st --platform a2a3simpytest examples tests/st --platform a5simpytest tests/ut -m "not requires_hardware"Not run on this branch: the full onboard a2a3 suite and the other four DFX
smokes. The dep_gen smoke is the one that reproduces this, and it was run across
eight widths; the rest is unverified here.
Follow-up, not fixed here
host_build_graphcallsdep_gen_aicpu_init()(
src/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_cold_path.cpp:899)but never calls
dep_gen_aicpu_set_orch_thread_idx()or..._flush(), sos_orch_thread_idxstays-1and anenqueue_readywould indexqueues[-1].Unreachable today — hbg has no dep_gen consumer path — but it is a separate bug
and wants its own change. Filing an issue.
Unblocks #1478 and #1309, both of which are red on this assertion.